feat: Add renderHook
#991
feat: Add renderHook
#991
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 144b485:
|
Codecov Report
@@ Coverage Diff @@
## main #991 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 169 181 +12
Branches 33 34 +1
=========================================
+ Hits 169 181 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I'm in support of this. What do you think @mpeyper? |
|
I have some thoughts on this, but not enough time at the moment to write it all up (starting work now). I'll come back to it tonight when the kids go to bed (about 12 hours from now). I'll tag @joshuaellis for his thoughts too. In the interim:
We are working on React 18 support, although admittedly it has not progressed much recently. I'm curious which APIs won't make sense in React 18, or are you are referring to React APIs (e.g. |
|
|
Yeah, I'm definitely not a fan of |
|
I think one of my major hesitations of this merge, is the loss for native support. As (and correct me if I'm wrong) I believe RTL supports Dom only? So we would be somewhat doing the community a disservice by removing a valuable feature we know people actively use. And on the topic of targets, would we be able to (and would we need to) support SSR rendering of the hooks, this was another major feature requested that we have since implemented. Whilst I understand that the API may not be compatible and some elements of the library go against the philosophy of TL would it perhaps not be better for the community if we make the changes in the actual library whilst maintaining existing functionality? I'm confused over the requirement to merge the two libraries or in this case duplicate functionality, is it that you think we at RHTL won't want to make these changes to become more uniform to the philosophy? |
We're not removing any feature that we think is valueable. As to actively being used: I looked at react-spectrum and apollo-client. Neither repo needs anything beyond
Like error boundaries, we can consider adding this to the renderer library. But I don't see why we need SSR for hooks but not components. Especially since we need to find a pattern first how to test SSR(+hydration) properly. All the examples I've seen just use SSR API in a browser which is rarely how your hook/component is used. The same applies to errors. This can be incredibly tricky to do right.
It just doesn't fit in the testing-library org. We have a library for different renderers. But then we also have a library for a specific implementation detail that works for multiple renderers? Regarding duplication: The goal is to reduce duplication. Right now RHTL needs to copy some testing utility from RTL and replay even more React implementation details if they want to be compatible with React 18. There's just too much inconsistent behavior enabled by the current setup. In the end, having |
|
Firstly, Secondly, I gather from your comments above that you're proposing this I'll approach this with the intent I hope it was raised from, which is providing the best tools for the users and try to address some of the points raised above and highlight any "lessons learned" from my time at the helm.
I think you are giving const currentResult = result.current;
await waitFor(() => {
expect(result.current).not.toBe(currentResult);
});For transparency, the implementation is not quite this, but due to the nature of hooks, it does function this way for the majority of custom hooks (they frequently return new object, array or function references). Perhaps it's my lack of understanding about what is to come in React 18, but I don't see why this would need to reimplement any batching logic or is Enzyme-esque in nature. In fact, the only compatibility issue we've had with react 18 so far seems to be that While I can see the concern about implementation details, I do feel like the intent of I will concede that I personally find The other main use case was for hooks using suspense. Again, they could use
One thing about our async utils that you wont get from using your So my take is that we haven't duplicated the utilities, but rather been inspired by them to write our own for our use case in the same way react-native-testing-library has written their own version, because the I actually wonder if perhaps there isn't space for a common async utils library that we could all collaborate on that has a more generic "something updated please check between intervals" implementation that
This is a concern for me too. The truth is that that the vast majority of custom hooks are not tied to react-dom or to the DOM at all. I feel like unless you can get
I disagree. RTL tests React components for web. RNTL tests React components for native. RHTL tests hooks for React Components. As I said above, hooks are not implicitly for DOM or Native (or whatever) components, especially in the library space were they might be expected to work anywhere. In fact, we moved to a "BYO renderer" model purely for convenience for our users that already had a perfectly good React renderer (
This bit confuses me. Your API has an option for hydration which I cant think of any other reason to have except for testing some kind of SSR functionality. The fact of the matter is that hooks behave differently on the server to how they do in the browser and where there is a difference in behaviour people are going to want to test that it behaves as they expect. If I was writing a hook in my library that I advertised as being SSR compatible I'd want to make sure I could back that claim with a test.
I think it's important to remember that our main target audience is library authors (and I consider common utilities in a project to be library-lite code), so the "software" being used is the custom hooks and not the react app using it. As such, there are inherent inconsistencies both with how people think about their test strategy and in how they use the software because they are different. Unless you are suggesting that only libraries that test user interactions are valid I've gone over this with you before and to a lesser extent with Kent, but for the benefit of this discussion I'll try to reiterate in context: Hooks are not components and component are not hooks (I hope we can all agree on this). Therefore, to suggest that the utilities you would use to test them must be completely consistent doesn't ring true to me (this might be where we start to disagree). Components are visual, interactive and user facing, while hooks are data focussed and code facing. In fact, I would go so far as to say that when testing a custom hook directly, the fact that is must be called within a component is an implementation detail we want to protect our users from, just like when you are testing a component directly, the fact that it is calling a hook is an implementation detail you want to protect your users from. After all we want to "test like your software is used" and our "software" is a function, not a component or an app. For example, there is not a great global object, i.e. the DOM, we can run assertions against so utilities like Another interesting example is errors. When a component throws an error the expectation is that it bubbles up to the nearest error boundary. It would be reasonable for you to expect your users to use the Basically what I'm saying is that by not supporting hooks as hooks, but just as extensions of components, you either leave a lot for users to implement as a Phew, that was a lot... I hope I didn't come across too defensive. I must admit, reading the comments above I definitely feel a little attacked as if somehow we're not doing our best to align ourselves to the ethos of I want to be clear though, I'm not completely against the idea of Now some other thoughts... I have long thought of const { result, rerender } = renderHook((message) => useSomeHook(message), { initialProps: 'hello' });
rerender('world');vs. let message = 'hello';
const { result, rerender } = renderHook(() => useSomeHook(message));
message = 'world';
rerender();The second example is clearer to me that I'll also add that I very rarely see Similarly, I find your use of Finally, users destructuring That is all for now. I hope this is all makes some sense from outside my head. I'm happy to clarify or dive deeper on any of it if you want. |
|
Oh, I forgot to mention const currentValue = selector();
await waitFor(() => {
expect(selector()).not.toBe(currentValue);
});The canonical use case for this is to wait for some loading flag to flip before running assertions: const { result, waitForValueToChange } = renderHook(() => useSomeRemoteData());
await waitForValueToChange(() => result.current.loading);
expect(result.current.someValue).toBe(...);This is a convenience utility and serves a similar purpose to your own Another note about our async utils that differ from the RTL ones is that, as hooks are primarily data based, we handle a few more cases with raw value instead or relying on assertions. In RTL you might do something like render(<Details />);
await waitFor(() => {
expect(screen.getByLabelText('Name')). toHaveTextContent('Michael');
});While in RHTL it would be something more like const { result, waitFor } = renderHook(() => useDetails());
await waitFor(() => {
expect(result.current.name).toBe('Michael');
});However, in the context of an data instead of components, it can often feel more natural and less verbose to write it like this: const { result, waitFor } = renderHook(() => useDetails());
await waitFor(() => result.current.name === 'Michael'));In RTL's This is another case where we have diverged from RTL in order to better tailor the tool in the context of hooks. This is probably also just an education piece instead of functionality you want to carry over, but I know people are using this variation and will be something you need to factor in. |
eps1lon
force-pushed
the
feat/renderHooks
branch
from
05193ce to
3d81844
Compare
Yep, that is my main concern. The secondary concern is that we don't have Put it another way: Why do we need
There isn't anything to handle for us. Suspense itself is an implementation detail for various concerns. What you should be testing is the behavior the user sees e.g. "1. click a thing 2. fallback shows 3. ??? 4. data shows". That's what you want to test. Not that a hook throws a promise that resolves eventually. Or that your hook renders for too long and results in React showing a fallback. We don't need any extra API for it because it's as simple as
Whether an intermediate state is painted is not an implementation detail. It is important to check to test for e.g. tearing, flashing etc.
It doesn't matter under the testing-library paradigm. We want to test what the user sees. Regarding API choices: I get the point of a React renderer agnostic library. Though I don't think the "hooks are mostly renderer agnostic" is very accurate. Similar points can be made about components. But we end up arguing about percentages and I don't think that discussion is very helpful
I want to emphasize that I find this to be generally a bad thing since it requires people to learn multiple approaches and shift between them when testing hooks vs components. By simplifying the API people can learn once and then apply that knowledge elsewhere. I want to caution against writing too specialized APIs for every specific testing need just so that it's convenient. Your point can just as well work as a counter argument since people might accidentally flip the patterns since they're not context aware of the thing they test. You have to constantly check whether you're testing a component or hook instead of just using |
|
I'm very much interested in tests that couldn't be implemented with this new API. So far I've looked through react-spectrum and apollo-client and repositories work pretty well with this new API. I'd even argue that the tests are more robust (across React 16-18). |
Just to be clear, "hooks are an implementation detail" for components is exactly why I believe Of course they're not. So now we have to adjust our thinking to test them appropriately for the characteristics and constraints that they present, some of which are consistent with components and some of which are not. And similarly, they are also very similar to regular functions, but we also can't test them as if they are standard functions, so we have to adjust out thinking and test them appropriately.
The short version is we probably don't. As described from both sides, in most cases it can be accomplished with
I'm not opposed to deprecating
No, I'm arguing the opposite. For a hook, there is no fallback state or too long or any of that component concern when the hook suspense. The only thing the hook needs to do is wait for the promise to resolve so it can continue rendering. And I 100% completely agree with you that adding these things doesn't make sense when testing a component because wrapping components is natural usage and makes sense in that context. In a component context, you do care and control what the user sees during while suspending so it makes sense to make that part of of your test, but in the hook context, there is nothing visual so making it part of the test's concerns just adds noise. Hooks that suspend while loading data is an increasingly common pattern and it's only going to become more common as time goes by. Even now, Every My philosophy has been to try to prevent the users of
My point was that when and how the value updates is not something that should leak out of the test and without the ability to run intermediary checks between intervals, we create situations where the timing matters and changes to the hook result in changes to the test which is something I want to avoid my users from doing. This is why our async utils run the checks again whenever the result changes, so they just work when the conditions of
Once again, you're approaching this with you component blinkers on. There is nothing for the user to see when you write a test with If the requirement for being a
Again, I find this whole feature to be against your own principles and beliefs. You want consistency across all the testing libraries, yet you're adding a feature that others are not, for a feature the others don't have. Why? Why is If you truely believe this then don't add I mean, the whole reason you are even proposing
This is the bit that really eats at me. What is so wrong with our implementation that it can't work in react 18? If you look through the messing around we do with multiple renderers, our renderHook looks very similar to yours. We're still rendering a component that calls the the hook and sticks the result into Yes, we wrap the test component automatically in a suspense and and error boundary, but that is possibly with Yes, we resolve a promise when If we removed
Let's look at a scenario described in the
So the sequence would be this:
Now imagine you want to test that the rejected error is thrown from your hook. And just to be clear, it's entirely possible you can get a passing test with the simplified API, but what I'm interested in from a In our version, this would be: import { renderHook } from '@testing-library/react-hooks';
import { useQuery } from '..';
test('should raise error when query fails', async () => {
const { result, waitFor } = renderHook(() =>
useQuery(['error-test'], Promise.reject(Error('Oh no!')), { suspense: true })
);
await waitFor(() => {
expect(result.error).toEqual(new Error('Oh no!'));
});
});I imagine with the simplified API, this test would look something like: import { renderHook, waitFor } from '@testing-library/react';
import { useQuery } from '..';
test('should raise error when query fails', async () => {
let caughtError;
class ErrorBoundary extends React.Component {
constructor(props) {
super(props);
this.state = { hasError: false };
}
static getDerivedStateFromError(error) {
return { hasError: true };
}
componentDidCatch(error, errorInfo) {
caughtError = error;
}
render() {
if (this.state.hasError) {
return null;
}
return this.props.children;
}
}
const wrapper = ({ children }) => (
<Suspense fallback={null}>
<ErrorBoundary>{children}</ErrorBoundary>
</Suspense>
);
renderHook(() =>
useQuery(['error-test'], Promise.reject(Error('Oh no!')), { suspense: true }),
{ wrapper }
);
await waitFor(() => {
expect(caughtError).toEqual(new Error('Oh no!'));
});
}); |
|
I'm afraid I don't have time to participate in this discussion (just got a new job), but I wanted to drop a note to say I appreciate you both putting so much tone into finding the solution that's best for the community regardless of any and all sunk costs. Whatever the best path forward is important (even if it means I have to add a note about changes in TestingJavaScript.com 😅). Thank you both for all your efforts here. It makes an enormous positive impact on the community and I trust you both to come to a good decision 👍 |
|
Thanks @kentcdodds and good luck with the new gig. I legitimately look forward to evaluating remix when it's released after following its development for so long. Honestly, I'm not sure we will come to a consensus here. Our views are just too different and we're both just reiterating our same points to each other, worded slightly differently hoping we can make the other understand. Truthfully, I think we both do understand each others points and there is just enough grey area in testing hooks that both sides are valid to some degree and the truth lies somewhere in the middle. I think this discussion would be very much improved with a few more voices and points of view. I do feel like this is going to happen whether I like it or not and now I need to decide just how much more effort I want to put into this discussion, my implementation and OSS in general (I've spoken in the past about feeling quite burnt out on it all, so this is not specifically triggered by this, just bringing it to a head). I don't mean that to sound passive aggressive (although I would understand if it is read that way), I just don't know how to word it any differently. @eps1lon, for the record, our entire test suite (which covers a lot more hooks and edge cases than the 2 tests in this PR) is passing with the |
|
And just as an aside, I'm also finding the rhetoric that "the API is different and so people will have to learn a new tool which is bad" to be largely unfounded and just your opinion on it. We get very few support tickets or questions in discord (bordering on none) about how to use our library. The vast majority is people trying to test their components with it and think we have some super secret magical API that can extract the internal values of hooks out of them, to which we redirect them to |
|
Maybe this would benefit from an unbiased RFC issue that we could share with the wider community to get feedback on. Although it would be good before doing that understanding the full scope of the proposal which to me (and feel free to correct me @eps1lon) is:
|
|
That's not a bad idea @joshuaellis. I'd be very interested in hearing the thoughts of the |
|
Absolutely, I can draft up a proposal and send it to you both in Discord to avoid curation noise in this PR if you're happy with that @eps1lon? |
eps1lon
force-pushed
the
feat/renderHooks
branch
from
225524d to
93a5b8e
Compare
eps1lon
force-pushed
the
feat/renderHooks
branch
from
93a5b8e to
78dbb26
Compare
Conceptually the state is pending to be committed. So this makes more sense than tying it to "render"
|
I would like to echo a desire to swap renderHook from react-hooks to react ahead of the full React 17 -> 18 migration. Is it something feasible to patch into a new 12.x release, or to be done as part of a yarn-patch? |
|
Hi |
|
Hi, I just spent some time moving a medium-sized (130k loc) TypeScript codebase to this new API in preparation for a React 18 upgrade, and just wanted to share my experience as a real-world example of the effects these API changes: https://github.com/foxglove/studio/pull/4063 (Note: this PR doesn't actually move from As I worked on these changes, it struck me as a bit unfortunate that in order to upgrade to React 18, in addition to changing the actual React API calls, I had to get sidetracked and change a bunch of tests that were using renderHook's Specifically the changes I made were roughly of this form:
None of the changes alone were particularly hard, but it made the code messier and more verbose. |
|
@mpeyper In your post last year (#991 (comment)) you mentioned that The original issue has some context on why this feature is useful (testing-library/react-hooks-testing-library#461) We are currently getting a lot of benefit out of asserting You mention the addition of We are investigating whether we can implement some wrapper around our hooks so that we can still assert all states. Or maybe reimplement the |
|
Hi @pvandommelen, That decision is entirely up to the maintainers of
Not only useful in the context SSR, just that before we had SSR rendering supported, it gave the ability to see what the initial value of the hook was before effects fired, as if it was SSR’d. I say it was a mistake because it often encourages people to put too much emphasis on the transient values of their hooks and ending up with brittle tests that take a more effort to maintain, when most of the time, the final value is all they really cared about. |
|
We have a situation in our app where testing the initial value is important. We have a hook that performs GET requests that can be enabled/disabled via a We had a bug in our code where the initial value was being set to false, but then quickly changed in an effect like this: const useRequest = ({ when, ...queryOptions }) => {
const [isFetching, setIsFetching] = useState(false)
const fetch = useEvent(...)
useEffect(() => {
if (when) setRequestId(generateUuid)
}, [when])
useEffect(() => {
if (!requestId) return
setIsFetching(true)
fetch(queryKey)
}, [fetch, queryKey, requestId])The brief time where isFetching is initially false is a big problem for places relying on that status to show loading state instead of content. The bug is easily fixed by changing the initial value of isFetching: const [isFetching, setIsFetching] = useState(!!when)Having test('sets isFetching to true', async () => {
const scope = nock(url).get('/').reply(200, JSON.stringify('OK'))
const { result, waitFor } = renderHook(() => useGet(url, options), {
wrapper: _.createCleanCache()
})
expect(result.all[0].isFetching).toEqual(true)
await waitFor(() => expect(scope.isDone()).toEqual(true))
})If you change this test to use What's the recommendation for testing this without So far, the best I have come up with feels much worse: test('sets isFetching to true', async () => {
const Component = () => {
const { isFetching } = useGet(url, options)
const initialIsFetching = useRef(isFetching)
return `initial isFetching: ${initialIsFetching.current}`
}
const Wrapper = _.createCleanCache()
render(
<FetchMock mocks={[{ url: '/', status: 200, response: 'OK' }]}>
<Wrapper>
<Component />
</Wrapper>
</FetchMock>
)
expect(screen.getByText('initial isFetching: true')).toBeInTheDocument()
}) |
|
@JohnColvin check out my comment at #991 (comment) for some examples of how to replace |
|
Thanks @jtbandes. I'll try your second example of tracking the But I'm hoping that my comment shows a case where the initial value is important as a vote to return some of the previous functionality of |
|
Thanks again @jtbandes! We've implemented something based on your approach and our tests are looking good now, with very little change. There's a small bug in your version posted above. The render function needs to be passed the props given to it. If you use the This is what we've done to wrap const customRenderHook = (renderFn, options) => {
const results = []
const renderFnWithResultHistory = props => {
const result = renderFn(props)
results.push(result)
return result
}
return {
results,
...renderHook(renderFnWithResultHistory, options)
}
}and to avoid handling the arguments to const renderFnWithResultHistory = R.pipe(
renderFn,
R.tap(result => results.push(result))
) |
I can relate that we are sadly facing the same issue migrating the https://github.com/openfun/richie repository from React 17 -> 18. Especially the case where props aren't shared anymore with the wrapper, which forces to use top-scoped variables as described in the great exemple you provided @jtbandes . In any case thank you for the work put in this library. |
|
I've written a small utility to aid in migrating import { waitFor } from '@testing-library/react';
const waitForValueToChange = async <T,>(getValue: () => T) => {
const original = getValue();
await waitFor(async () => {
expect(await original).not.toEqual(await getValue());
});
}; |
|
For anyone else that gets here looking for migration help, there is a WIP migration guide here. Any contributions to it with breaking changes, advice, clarifications and/or shims are more than welcome. |
What:
Imlement minimal
renderHookfrom@testing-library/react-hooksWhy:
@testing-library/react-hooksis not compatible with React 18 and it's questionable if certain APIs will make sense in React 18.This ensures that the supported version of React that is used with your Testing Library APIs is always consistent.
error testing: We don't have this solved in
@testing-library/react. It should be solved in the renderer testing library.suspense: Can be solved with
wrapperwaitForNextUpdate: UsewaitForinstead. When an update is triggered and what is part of an update is not always clear and in 99% of the cases (even if hooks relying on data e.g. apollo-client) not relevant. See eps1lon/apollo-client#1How:
Implement minimal
renderHookwith justrerender,rerenderandunmount. Assertions are made on the committed result not the render result.Renders are not something to test for since they're ultimately not detereministic depending on the component implementation. However, we don't want to test component implementation details.
For more granular testing one could always create a custom test component around the hook under test and pass it to our
render. This has always been the recommended approach anyway. But it requires more writing and instead of insisting that this is bad practice we meet devs halfway, provide a render wrapper around creating a test component but then reduce the API of that render wrapper.If your goal was to count renders then
React.Profileris the better approach anyway.Checklist:
docs site: docs:
@testing-library/react#renderHooktesting-library-docs#967